OpenShift Support: Product Telemetry #4038
Conversation
github-actions
bot
added
enhancement
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4038 +/- ##
==========================================
+ Coverage 86.69% 86.70% +0.01%
==========================================
Files 128 128
Lines 16758 16764 +6
Branches 62 62
==========================================
+ Hits 14528 14535 +7
+ Misses 2045 2044 -1
Partials 185 185 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cmd/gateway/commands.go
Outdated
| imageSource = "unknown" | ||
| } | ||
|
|
||
| buildOs := os.Getenv("BUILD_OS") |
There was a problem hiding this comment.
Couldn't you just call this directly in the collector, rather than calling here and passing the var all the way through the code?
There was a problem hiding this comment.
That's a great point! Since I saw the same for imageSource := os.Getenv("BUILD_AGENT") above I figured this was the appropriate way to approach it.
* Update module google.golang.org/grpc to v1.76.0 | datasource | package | from | to | | ---------- | ---------------------- | ------- | ------- | | go | google.golang.org/grpc | v1.75.1 | v1.76.0 | Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Update files for renovate --------- Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
| datasource | package | from | to | | ---------- | ---------------------------- | ------- | ------- | | go | github.com/prometheus/common | v0.66.1 | v0.67.1 | Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
| datasource | package | from | to | | ----------- | -------------------- | ------- | ------- | | github-tags | github/codeql-action | v3.30.6 | v4.30.7 | Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* Update nginx Docker tag to v1.29.2 | datasource | package | from | to | | ---------- | ------- | ------ | ------ | | docker | nginx | 1.29.1 | 1.29.2 | Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Update README * Remove package updates --------- Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Ciara Stacke <[email protected]>
…38dc (#4039) Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
github-actions
bot
added
documentation
|
@shaun-nx also looks like there is some rebase issue occurring since theres a few additional files |
github-actions
bot
added
tests
| // | ||
| //go:generate go run -tags generator github.com/nginx/telemetry-exporter/cmd/generator -type=Data -scheme -scheme-protocol=NGFProductTelemetry -scheme-df-datatype=ngf-product-telemetry | ||
| type Data struct { | ||
| type Data struct { //nolint //required to skip golangci-lint-full fieldalignment |
There was a problem hiding this comment.
why do you need nolint here?
There was a problem hiding this comment.
The golangci linter sets a limit on the number of characters allowed in a struct. I can't remember what the exact error was, but it was related to fieldalignment. If I remember correctly the linter errored as we could have had less pointer bytes overall (not 100% sure what that means)
It resolved it by remving the comments around the fields in the struct. The issue with that those comments are required as part of the go generate command when we are generating the data.avdl file for telemetry
There was a problem hiding this comment.
That's not quite right. The fieldalignment linter helps to optimize Go structs so that they take up less memory in the binary at compilation. The linter automatically fixes these issues by reorganizing the struct to fit the most optimal order, but there's a bug that causes the comments to be removed.
We don't want to nolint here. The solution is either let the linter reorganize, and then manually add the comments back in, or in this case you can probably satisfy the linter by manually putting the BuildOS string alongside the other strings in the struct, because having the same types next to each other helps with the optimization. But we should not ignore these with a nolint.
There was a problem hiding this comment.
@sjberman the issue is that the data.avdl file is auto-generated by this Data struct from the telemetry-exporter. And we can't change the order of the data.avdl elements.
There was a problem hiding this comment.
Ah I see, thanks for the clarification.
5 participants
Proposed changes
This change adds the
BuildOSfield to our product telemetry.Closes #4037
Blocked by #4008
Should not be merged until feat/inference-extension is merged
Checklist
Before creating a PR, run through this checklist and mark each as complete.
Release notes
If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.